Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a Parser Idempotency Issue #3172

Merged
merged 2 commits into from
Dec 4, 2023
Merged

Fix a Parser Idempotency Issue #3172

merged 2 commits into from
Dec 4, 2023

Conversation

veera-sivarajan
Copy link
Contributor

This PR fixes #3133 by correctly implementing ToIndentedString for with statement.

This also replaces the existing ToInternedString implementation because it is implemented for all types implementing ToIndentedString in boa_interner/src/lib.rs.

@codecov
Copy link

codecov bot commented Jul 25, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eb2f33e) 44.63% compared to head (5bcaf27) 44.64%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3172   +/-   ##
=======================================
  Coverage   44.63%   44.64%           
=======================================
  Files         487      487           
  Lines       50554    50554           
=======================================
+ Hits        22563    22568    +5     
+ Misses      27991    27986    -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@raskad
Copy link
Member

raskad commented Jul 25, 2023

Nice fix!
@veera-sivarajan do you think it makes sense to add a test for this? I think this was only discovered via fuzzing right?

@raskad raskad added this to the v0.18.0 milestone Jul 25, 2023
@raskad raskad added bug Something isn't working parser Issues surrounding the parser labels Jul 25, 2023
@veera-sivarajan
Copy link
Contributor Author

Yeah, this was discovered by the fuzzer but I'm not sure if it requires an additional test because the additional braces were added by how the with statement was formatted ({{}} instead of {}) in to_interned_string() which, as far as my rust-analyzer could parse, is called only by the fuzzer?

@Razican
Copy link
Member

Razican commented Jul 26, 2023

We should still have a test, to make sure that we don't introduce a regression in the future.

@jedel1043
Copy link
Member

jedel1043 commented Nov 29, 2023 •
edited
Loading

Waiting on adding the required test to merge this. @raskad will take care of that.

veera-sivarajan and others added 2 commits December 4, 2023 00:25
@veera-sivarajan @raskad
Fix a Parser Idempotency Issue
1867f4b 
This PR fixes boa-dev#3133 by correctly implementing `ToIndentedString`
for `with` statement.

It also replaces the existing `ToInternedString` implementation
because it is implemented for all types implementing
`ToIndentedString` in `boa_interner/src/lib.rs`.
@raskad
Add test for with statement formatting
5bcaf27
jedel1043
jedel1043 approved these changes Dec 3, 2023
View reviewed changes
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@raskad raskad requested a review from a team December 4, 2023 00:14
@HalidOdat HalidOdat added this pull request to the merge queue Dec 4, 2023
Merged via the queue into boa-dev:main with commit 1fe164b Dec 4, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HalidOdat HalidOdat HalidOdat approved these changes

@jedel1043 jedel1043 jedel1043 approved these changes

Assignees

@raskad raskad

Labels
bug Something isn't working parser Issues surrounding the parser
Projects
None yet
Milestone
v0.18.0
Development

Successfully merging this pull request may close these issues.

Parser Idempotency issue: braces added every time after each pass
5 participants
@veera-sivarajan @raskad @Razican @jedel1043 @HalidOdat